fix(isthmus): adding support for new proto and deprecated form in AggregateRel #521
Conversation
… of Grouping for the AggregateRel
|
|
…r the AggregateRel
There was a problem hiding this comment.
For future readers, the context of this change arise from:
substrait-io/substrait#700
substrait-io/substrait#706
Thanks for starting on this. It would be good to have some tests for this. I would suggest adding a new test class like AggregateRelTest extends TestBase in this directory. Things to test would include consuming a proto plan with only the deprecated aggregate encoding and consuming a proto plan with the new aggregate encoding.
Beyond this, it might be good to consider updating the RelProtoConverter to output the new format as well For migration purposes, we should set both the new fields alongside the existing ones so that consuming systems can use whichever they support. This might require updating the Aggregate class itself to better align with the new representation.
…oto in RelProtoConverter
…of proto in RelProtoConverter
…ithub.com:gord02/substrait-java into gordon.hamilton/aggregateGroupingNewSubstraitForm
… of Grouping for the AggregateRel
…r the AggregateRel
…of proto in RelProtoConverter
vbarua
force-pushed
the
gordon.hamilton/aggregateGroupingNewSubstraitForm
branch
from
dfd50eb to
0a13b15
Compare
core/src/main/java/io/substrait/relation/ProtoRelConverter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/substrait/relation/ProtoRelConverter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/substrait/relation/RelProtoConverter.java
Outdated
Show resolved
Hide resolved
…multi grouping set test
…ithub.com:gord02/substrait-java into gordon.hamilton/aggregateGroupingNewSubstraitForm
core/src/main/java/io/substrait/relation/ProtoRelConverter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/substrait/relation/ProtoRelConverter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/substrait/relation/ProtoRelConverter.java
Outdated
Show resolved
Hide resolved
Substrait is either produced using using the deprecated form where Groupings is set with a grouping_expressions list, or the new proto form where the grouping_expressions is defined on the AggregateRel itself with the Groupings having a list of references into the grouping_expressions list.
This PR would add support for both of these forms resolving bugs that expected the deprecated form of AggregateRel.
Key Changes: